Previous code did not survive the 3-month later check; I was confused by the reassignment of variables stemming from my attempts to limit the number of constants.

I've gone the other way now with this PR and creating single-use constants that better reflect the individual parts of the path, which can be assembled into an object implementing the interface at end.

This file defines the paths where the test files are located.

Test writers should import the appropriate const and pass it into the createTestFile method.

All other methods for generating test files have been removed. Test writers are expected to pass the path of the source image file to the createTestFile method.

This file has been changed to TypeScript.

If the constants.js file is located within specs/specs-playwright, it causes Jest to attempt to execute the file as a test. This then fails due to the constants.js file not being a test.

Could you explain what are we trying to do here? Seems weird to open a file descriptor and not use it (plus we should close the fd later).

Is the goal to check that the file exists and is readable? In that case maybe we can use fs.access()

The intent is to check whether the file exists first.
With that said, it seems something else is swallowing the error - an invalid path given to this method didn't output stack trace as I would have expected.

It looks like throw Error is being swallowed since the code calling createTestFile is within a hook.

I've changed the throw statements to instead console.error and have verified this isn't swallowed up:

  console.error
    Source file /Users/edwintakahashi/workspace/wp-calypso-third/test/e2e/specs/image-uploads/image0.jpg not found on disk.

      80 | 		await fs.access( sourcePath );
      81 | 	} catch {
    > 82 | 		console.error( `Source file ${sourcePath} not found on disk.` );
         | 		        ^
      83 | 		// throw new Error( `Source file ${ sourcePath } not found on disk.` );
      84 | 	}
      85 |

      at Object.createTestFile (../../packages/calypso-e2e/src/media-helper.ts:82:11)
      at specs/specs-playwright/wp-blocks__media-spec.ts:27:11

This is a common topic of discussion as shown here.

Fixed in 5d4b3c3.

fs.mkdtemp() "Creates a unique temporary directory. A unique directory name is generated by appending six random characters to the end of the provided prefix" (from the docs).

Do we still need to append a random integer to the filename?

The uniqueness here isn't about the directory/path per se, but rather has to do with the filename portion.

If two tests both end up creating a test file named identical_filename.jpg, uploading this to WPCOM would result in one instance of the file being renamed with a -1 postfix.
Later down the test, there are checks for whether the expected file name is found on the page. The -1 postfix interferes with this.

Another solution is to switch the validation to use a regex to match the important portion of the file name eg. 167888888.*.jpg.

Removed the postfix in cbd61f3.

Maybe use mode COPYFILE_EXCL to fail if the target already exists.

Added in 5d4b3c3.

The goal of path.resolve() is to find an absolute path appending from right to left, I think is misused here.

Instead we can use any of these:

path.join(__dirname, '../image-uploads/image0.jpg')
path.normalize(`${__dirname}/../image-uploads/image0.jpg`)

I see, I had assumed the resolve would mean it joins all of them then resolves the resulting string to a path on the disk. Not the best name in my opinion.

I will choose path.normalize(`${__dirname}/../image-uploads/image0.jpg`).

Changed to a combination of path.normalize(path.join()) in 5d4b3c3.

We don't want to log the error in case of test_fn_failure? Does jest does it automatically?

At the very least I think we should set this.global.__CURRENT_TEST_FAILED__ = true; here. That global property is checked by the screenshot/video recorder hooks.

Most of the time a failed hook mean there is nothing to take a screenshot from, but we can't guarantee that will be the case forever (for example, we may have a hook that opens the browser and does some navigation, and we do want to screenshot that if it fails).

I think my comment also went missing too 👀

We don't want to log the error in case of test_fn_failure? Does jest does it automatically?

In our current configuration, Jest automatically throws and outputs the relevant logs if the error is thrown outside of a hook.

At the very least I think we should set this.global.CURRENT_TEST_FAILED = true; here. That global property is checked by the screenshot/video recorder hooks.

Avoiding the screenshot/video recording codepath was why initially the value was not set. But as you mention:

we can't guarantee that will be the case forever (for example, we may have a hook that opens the browser and does some navigation, and we do want to screenshot that if it fails)

Yes, that is true. I will treat hook failure like a normal test failure in next commit.

Fixed in 0581a7d.

Revised behavior now outputs the following if anything in the hook fails:

 FAIL  specs/specs-playwright/wp-media__edit-spec.ts
  ● Test suite failed to run

    Source file /Users/edwintakahashi/workspace/wp-calypso-second/test/e2e/image-uploads/image0.jpg not found on disk.

      78 | 		await fs.access( 'sourcePath' );
      79 | 	} catch {
    > 80 | 		throw new Error( `Source file ${ sourcePath } not found on disk.` );
         | 		      ^
      81 | 	}
      82 |
      83 | 	// Obtain the file extension.

      at Object.createTestFile (../../packages/calypso-e2e/src/media-helper.ts:80:9)
      at specs/specs-playwright/wp-media__edit-spec.ts:22:15

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        2.705 s

But I suspect throw event.error will prevent rest of the beforeAll or afterAll hook from executing, so this will likely not capture screenshots.

I've revised the behavior again in 87006be since I felt the previous behavior was a bit hacky.

Recap: throwing an error in environment.js causes rest of execution to halt. Practically, this means the afterAll hooks of screenshot/video capture are not run.

I've changed the behavior to instead do the following:

1. if a hook failure is detected, set a new attribute hookFailed.
2. at test_start event, check if hook has failed.
3. if hook has failed, mark all test cases in the describe block as failed.

I arrived at this solution because these lines were causing hook failures to be treated like a normal skip and ended up suppressing the errors thrown. To verify this, add an exception in a hook, modify the skip to a fail. Errors thrown in the hook should now be visible.

The separate treatment of hook failure is required, otherwise if an exception is thrown in the test step it would not be skipped. This can be verified by performing the same steps as above, but place the exception in the test step instead. Jest will not skip rest of the test steps.

Try pulling my changes in this revision, place exceptions in beforeAll hook, test step and afterAll hook. It should behave following this rule:

1. if exception is thrown in a hook, suite is marked as failed and test steps yet to run are also marked as failed.
2. if exception is thrown in a step, only the failing step is marked failed and rest are skipped.

Scenario 1:

  ● [WPCOM] Media: Edit Media: (desktop) @parallel › Edit Image (Atomic) › Cancel image edit

    hello!

      20 |
      21 | 	beforeAll( async () => {
    > 22 | 		throw new Error( 'hello!' );
         | 		      ^
      23 | 		testImage = await MediaHelper.createTestFile( TEST_IMAGE_PATH );
      24 | 	} );
      25 |

      at specs/specs-playwright/wp-media__edit-spec.ts:22:9

Test Suites: 1 failed, 1 total
Tests:       16 failed, 16 total
Snapshots:   0 total
Time:        3.14 s, estimated 12 s

Scenario 2:

  ● [WPCOM] Media: Edit Media: (desktop) @parallel › Edit Image (Simple) › Log In

    hello!

      31 |
      32 | 		it( 'Log In', async function () {
    > 33 | 			throw new Error( 'hello!' );
         | 			      ^
      34 | 			const loginFlow = new LoginFlow( page, user );
      35 | 			await loginFlow.logIn();
      36 | 		} );

      at Object.<anonymous> (specs/specs-playwright/wp-media__edit-spec.ts:33:10)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 15 skipped, 16 total
Snapshots:   0 total
Time:        3.078 s, estimated 4 s

Scenario 3:

  ● Test suite failed to run

    hello!

      60 |
      61 | 		afterAll( async function () {
    > 62 | 			throw new Error( 'hello!' );
         | 			      ^
      63 | 		} );
      64 | 	} );
      65 | } );

      at specs/specs-playwright/wp-media__edit-spec.ts:62:10
          at runMicrotasks (<anonymous>)

Test Suites: 1 failed, 1 total
Tests:       2 passed, 2 total
Snapshots:   0 total
Time:        11.912 s, estimated 43 s

@scinos Could I please get a review on this before it gets too stale? Thanks!